Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] Adding feature 'Gaussian Filter' #446

Closed
wants to merge 3 commits into from
Closed

[WIP] Adding feature 'Gaussian Filter' #446

wants to merge 3 commits into from

Conversation

laxsuryavanshi
Copy link
Contributor

@laxsuryavanshi laxsuryavanshi commented Mar 9, 2020

Description

Gaussian Filter

The Gaussian smoothing operator is a 2-D convolution operator that is used to `blur' images and remove detail and noise.

References

https://homepages.inf.ed.ac.uk/rbf/HIPR2/gsmooth.htm

Further Development during GSoC

  • Add more algorithms
  • Documentation of Code
  • Review and approval from mentor and merge of this PR

Tasklist

  • Add test case(s)
  • Ensure all CI builds pass
  • Review and approve

@mloskot mloskot added status/work-in-progress Do NOT merge yet until this label has been removed! google-summer-of-code All items related to GSoC activities labels Mar 10, 2020
@lpranam
Copy link
Member

lpranam commented Mar 10, 2020

Please stop creating new PR every time for same thing after modification

@mloskot
Copy link
Member

mloskot commented Mar 10, 2020

Yes, good point. I also wondered what's that about but did not checked it in details, just labelled.

@mloskot
Copy link
Member

mloskot commented Mar 10, 2020

Yes, I asked for separate PR fixing a typo, but it should be handled quite easily and without necessity of separate PR.

since I commited everything on develop branch, I find it difficult to separate out

Before re-creating PRs, you could ask how to do it, how to extract change from existing PR to submit as separate PR. Chances are, somebody would help you and you could have learned it for future situations that may be common during GSoC as well as during your activities on GitHub generally.

Copy link
Member

@lpranam lpranam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think gaussian kernel generation requires a separate PR. You also need to submit a test for Gaussian kernel first.

@mloskot
Copy link
Member

mloskot commented Mar 10, 2020

In one of other PRs, I think I suggested to add a utility converting between 1D and 2D kernels if separability is needed but 2D kernel is in hand. Just reminding the idea, in case it's a good idea. There may be better ways to solve it perhaps.

@lpranam
Copy link
Member

lpranam commented Mar 10, 2020

@mloskot I thought of that and after some research, I came to know that directly creating 1d kernel can be performance efficient

@mloskot
Copy link
Member

mloskot commented Mar 10, 2020

@lpranam The kernel conversion idea follows your #443 (comment) about re-using this function

inline detail::kernel_2d<T, Allocator> generate_gaussian_kernel(std::size_t side_length, double sigma)

If this function can not be reused or its outcome can not be (efficiently) converted to usable form, perhaps this function should be reworked to generate 1D or 2D kernels.

My motivation is to look for possible pattern common for number of use cases to avoid breeding too many (public!) generator functions, especially for the same filters. If we could have some neat common D-agnostic interface for kernel generators...


@laxsuryavanshi If I may, I'd suggest you to focus more on your GSoC proposal than contributing new features to GIL before GSoC starts. If you participate in GSoC, you will have time to get those features well designed and implemented. I understand you may be eager to sling PRs with new features and ideas, but this is not the best time now to propose significant features, before GSoC.
As you see, it takes time to discuss and agree on things and we don't have that time now, so your PRs will likely remain unanswered.

To summarise,

@mloskot
Copy link
Member

mloskot commented Mar 10, 2020

I'd like to contribute to GIL even if I didn't selected cause this will only increase my programming skills.

Great and you're welcome to contribute. It's just that it usually takes time until a contributor becomes familiar with any non-trivial codebase, so her/his is capable to propose new features designed and implemented so that they fit the whole picture well.

p.s. FYI, Sir is not necessary really :)

@lpranam
Copy link
Member

lpranam commented Mar 11, 2020

@lpranam The kernel conversion idea follows your #443 (comment) about re-using this function

That's when @laxsuryavanshi chooses to use 2D Gaussian kernel but if he chooses to use 1D Gaussian kernel a newer implementation can generate 1d kernel faster then converting from 2d to 1d.

My motivation is to look for possible pattern common for number of use cases to avoid breeding too many (public!) generator functions, especially for the same filters. If we could have some neat common D-agnostic interface for kernel generators...

I think we can have overloads for 1D and 2D kernels which can be handy for the user.

@mloskot
Copy link
Member

mloskot commented Mar 11, 2020

Yes an overload taking in/out kernel parameter sounds fine.

I'm also a bit allergic to sprinkling get_* free functions over the public interface.

template <typename T = float, typename Allocator = std::allocator<T>>
kernel_1d<T, Allocator> get_1d_kernel_from(

Even for such a simple functionality, we should be quite considerate how to shape the interface of it, also because of https://martinfowler.com/bliki/TwoHardThings.html, unless it's going to live inside namespace gil::detail, then we can afford sloppy.

@mloskot mloskot removed the status/work-in-progress Do NOT merge yet until this label has been removed! label Sep 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
google-summer-of-code All items related to GSoC activities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants